Conversation
da50e0d to
c896461
Compare
c896461 to
d864085
Compare
Deploying openproject with ⚡ PullPreview
|
0243a84 to
b94a116
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces the legacy Backlogs sprint taskboard (server-rendered view + legacy JS/CSS) with an automatic redirect to a Work Package Board, creating the board on demand for a sprint.
Changes:
- Add
TaskBoards::CreateServiceto ensure a sprint board exists (create if missing, handle RecordNotUnique), plus unit specs. - Change
RbTaskboardsController#showto create/load the board and redirect toproject_work_package_board_path, with a localized failure flash. - Remove legacy taskboard view layer, feature/view specs, legacy frontend assets (Stimulus “taskboard-legacy” stack + vendor jQuery plugins + Sass), and drop
jquery.cookiefrom frontend dependencies.
Reviewed changes
Copilot reviewed 30 out of 33 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
modules/backlogs/app/controllers/rb_taskboards_controller.rb |
Redirects sprint taskboard route to the generated board, with error fallback. |
modules/backlogs/app/services/task_boards/create_service.rb |
Implements board + per-status query/widget creation and an idempotent .ensure entrypoint. |
modules/backlogs/app/models/sprint.rb |
Adds #board_name used to name/lookup the sprint board. |
modules/backlogs/config/locales/en.yml |
Adds translation for automatic board creation failure. |
modules/backlogs/spec/services/task_boards/create_service_spec.rb |
New specs for the board creation service. |
modules/backlogs/spec/controllers/rb_taskboards_controller_spec.rb |
Updates controller spec to expect redirect behavior and failure handling. |
modules/backlogs/spec/models/sprint_spec.rb |
Adds spec coverage for Sprint#board_name. |
modules/backlogs/app/views/rb_taskboards/show.html.erb |
Removes legacy taskboard view template. |
modules/backlogs/app/helpers/taskboards_helper.rb |
Removes legacy helper used only by the removed view. |
modules/backlogs/spec/views/rb_taskboards/show_spec.rb |
Removes legacy view spec. |
modules/backlogs/spec/features/tasks_on_taskboard_spec.rb |
Removes legacy JS feature spec for tasks on taskboard. |
modules/backlogs/spec/features/impediments_spec.rb |
Removes legacy JS feature spec for impediments on taskboard. |
frontend/src/stimulus/controllers/dynamic/backlogs/* (multiple) |
Removes legacy taskboard JS implementation. |
frontend/src/vendor/jquery.* (multiple) |
Removes vendor plugins used by the legacy taskboard implementation. |
frontend/src/assets/sass/backlogs/* (multiple) |
Removes legacy taskboard/statistics/global styles and related imports. |
frontend/package.json / frontend/package-lock.json |
Removes jquery.cookie dependency. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
You can also share your feedback on Copilot code review. Take the survey.
modules/backlogs/spec/services/task_boards/create_service_spec.rb
Outdated
Show resolved
Hide resolved
d9d757d to
4b0f8eb
Compare
400c7c4 to
1598c02
Compare
|
@ulferts while this PR still needs some polishing, as well as feature specs, if you could give this a quick sanity check, that would be immensely helpful! |
3b5e2cd to
6b1b9d4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
You can also share your feedback on Copilot code review. Take the survey.
modules/backlogs/spec/components/backlogs/sprint_menu_component_spec.rb
Outdated
Show resolved
Hide resolved
modules/boards/app/services/boards/sprint_task_board_create_service.rb
Outdated
Show resolved
Hide resolved
601636f to
50b109c
Compare
These are commands, not partial updates, so POST is the correct HTTP method semantically.
Extracts failure aggregation into its own method.
The `.visible` scope returns 404 before `authorize` runs, so expects `:not_found` instead of `:forbidden` for members without permission.
Introduces `Sprints::StartContract` inheriting from `BaseContract` to validate permissions, sprint status, and active sprint uniqueness at the service layer. Refactors `Sprints::StartService` to use `BaseServices::BaseContracted` with contract integration.
fd1c67c to
39b018e
Compare
Introduces `Agile::Sprint.receiving_projects(sprint)` scope returning all projects with work packages on the sprint. Updates `ensure_task_boards` to create boards for all receiving projects. Moves `board_name` from model to service.
The `load_sprint_and_project` callback already calls `load_project` for start/finish actions. Including `SPRINT_STATE_ACTIONS` in the separate `load_project` before_action caused it to run twice per request.
`Status.where(id:)` does not preserve the order of the input IDs. Extracts `statuses_in_order` to look up statuses by ID and return them in the widget column order from the previous sprint board.
39b018e to
e6a6ba0
Compare
Merges identical `respond_with_start_failure` and `respond_with_finish_failure` into a single method. Extracts `fallback_responses_for` following the pattern in `Projects::StatusController`.
|
Update 22.03.26 Here are some of the assumptions relating to the last couple commits:
|
modules/backlogs/app/components/backlogs/sprint_menu_component.rb
Outdated
Show resolved
Hide resolved
modules/backlogs/app/components/backlogs/sprint_header_component.rb
Outdated
Show resolved
Hide resolved
|
|
||
| def start | ||
| return render_403 unless OpenProject::FeatureDecisions.scrum_projects_active? | ||
| return render_404 unless @sprint.in_planning? |
There was a problem hiding this comment.
I personally don't see the benefit of doing this here. The loading of the sprint itself will already return a 404 if the sprint itself is not visible.
The routing constraint to me offers no benefits on top of that. This is a post/patch action so no information to glean from it anyway. But this is semantics so I accept that there are different opinions on this one and adapting is not a prerequisite for merging.
Depending on what is decided in today's weekly on where a sprint can be activated/finished from, the first guard might have to go.
modules/backlogs/spec/models/agile/sprints/scopes/receiving_projects_spec.rb
Show resolved
Hide resolved
modules/boards/app/services/boards/sprint_task_board_create_service.rb
Outdated
Show resolved
Hide resolved
| type: "action", | ||
| attribute: "status", | ||
| highlightingMode: "priority", | ||
| filters: [{ sprint_id: { operator: "=", values: [params[:sprint].id.to_s] } }] |
…ask-boards # Conflicts: # modules/backlogs/app/controllers/rb_sprints_controller.rb # modules/backlogs/spec/components/backlogs/sprint_menu_component_spec.rb # modules/backlogs/spec/features/sprints/edit_spec.rb # modules/backlogs/spec/support/pages/backlogs.rb
Dev merge moved sprint menu helpers from `Pages::Backlogs` to `Pages::SprintPlanning`. Updates the spec to match.
Moves `must_be_in_planning` and `only_one_active_sprint_allowed` from core `config/locales/en.yml` to the backlogs module's locale file, as these are sprint-specific error messages.
Removes the `project: sprint.project` default parameter from `SprintHeaderComponent#initialize`. All callers already pass `project:` explicitly, so the default was unnecessary and could mask missing arguments. Updates the component spec to pass `project:` explicitly.
Drops the `.active` scope from `shared_receiver_projects` in `ReceivingProjects`. The visibility of archived projects is already handled at the controller/view layer, so filtering here is redundant and can cause incorrect results. Adds spec coverage for the `share_subprojects` ancestor blocking behaviour when the source shares globally.
Replaces the map-then-aggregate pattern with a simpler loop that calls `add_dependent!` on a single `ServiceResult`. Since `add_dependent!` automatically propagates failure via `merge_success!`, no separate aggregation step is needed.
f07cb72 to
10ca498
Compare
Moves the inline `@sprints.select(&:active?).map(&:id)` computation from the ERB partial into `load_backlogs` in the controller. Passes `@active_sprint_ids` to both the agile list and sprint planning list partials.
Uses `params[:project].type_ids` instead of `Task.type` when the sprint has no work packages. This avoids the dependency on the global backlogs Setting and uses the types actually enabled for the project. Removes the now-unnecessary `Setting.plugin_openproject_backlogs` stubs from both service specs and ensures projects are created with the required types.
modules/backlogs/app/components/backlogs/sprint_menu_component.rb
Outdated
Show resolved
Hide resolved
Allows start and finish from receiving projects while keeping the command permission on the sprint source project. Starting still requires board access in the rendered project because success redirects to that project's board. Adds `can_start?` and `can_start_or_finish?` class methods to `StartContract` so permission logic is shared between the controller and `SprintMenuComponent`. Uses `deny_access` instead of `render_403` to handle anonymous users correctly.
Replaces five inline subqueries for the source project's id, lft, and rgt with a single `WITH source AS (...)` CTE. Uses Rails' `.with()` method already established in the codebase.
Ticket
https://community.openproject.org/wp/69139
https://community.openproject.org/wp/72942
What are you trying to accomplish?
Automatically create and configure a sprint task board when a user starts a sprint, so the team can immediately begin tracking work without manual board setup.
This branch also adds a matching skeleton
Finish sprintflow for symmetry. For now, finishing a sprint only updates sprint status and does not change board behavior.Important
There are some caveats:
WorkPackageFilterValues).404 Not Founderror on visiting the board via the sprint action menu's "Task board" item. If finished sprints were not filtered out in the UI as it currently stands, it might be possible to allow a workaround (e.g. to finish and restart the sprint).Screenshots
Explicit sprint start / finish (WP #72942)
What approach did you choose and why?
I split the work into sprint board creation and an explicit
Start sprintflow (OP #72942). The latter replaces lazy board creation on taskboard access, so board creation now follows sprint start and can use its own permissions.Board linkage took two iterations. I first tried storing origin metadata in
Boards::Grid#options, but that was awkward becauseoptionsis YAML in a text column, not JSONB. I then switched to a nullable polymorphic link onBoards::Grid, which gives stable lookup without relying on serialized metadata.This still leaves room for a narrower relation later. If sprint boards remain a dedicated concept, a direct
sprint_idcould be preferable for stronger semantics and proper referential integrity.I also added a matching
Finish sprintaction and service for symmetry with the new start flow. At this stage (and since the behaviour with associated boards is not yet defined), it is essentially a stub - only transitioning sprint status.What this PR does not do
Caveats: see above
Clean up and optimisations: although these PRs relate to this feature, these steps are technically out-of-scope / need their own review and can be probably be considered optional.
Merge checklist
Acceptance Criteria
Automatic Board Creation
Default Board Configuration
Board Visibility & Access
Preparatory & Validation